Implement drop-on-canvas + linkconnectoradapter consolidation #5898
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: Implement drop-on-canvas + linkconnectoradapter consolidation (#5898)
Impact: 469 additions, 240 deletions across 15 files
Issue Distribution
- Critical: 0
- High: 0
- Medium: 4
- Low: 3
Category Breakdown
- Architecture: 1 issues
- Security: 0 issues
- Performance: 2 issues
- Code Quality: 4 issues
Key Findings
Architecture & Design
This PR implements a significant refactor of the link interaction system with good architectural principles:
- LinkConnectorAdapter consolidation: Good move to eliminate duplicate state by wrapping the live canvas linkConnector rather than creating new instances
- Drop orchestrator pattern: Clean separation of concerns with specialized functions for slot and node surface candidate resolution
- Canvas pointer event abstraction: Well-designed abstraction for canvas coordinate conversion and pointer history
The WeakMap usage for adapter caching follows a reasonable pattern, though the garbage collection behavior might cause unexpected adapter recreation.
Performance Impact
The RAF-based pointer movement processing is well-architected but could benefit from optimization:
- Heavy DOM querying operations on every animation frame
- Unbounded pointer history growth without cleanup mechanisms
- Consider debouncing expensive operations or pre-caching hover targets
Bundle impact appears minimal with mostly refactoring existing functionality.
Integration Points
- Backward compatibility: Preserved existing Vue composable behavior patterns
- Extension compatibility: Maintained existing public APIs that extensions depend on
- Test coverage: Excellent Playwright test coverage for the new shift-drop functionality
Positive Observations
- Comprehensive E2E test coverage including edge cases and error scenarios
- Clean TypeScript interfaces and proper type safety throughout
- Good use of caching patterns to avoid repeated expensive operations
- Proper cleanup patterns with pointer history management
- Well-structured composable following Vue 3 best practices
References
- Repository Architecture Guide
- Vue 3 Composition API patterns
- ComfyUI Frontend Guidelines (CLAUDE.md)
Next Steps
- Address medium priority issues for better robustness (pointer history cleanup, number validation)
- Consider performance optimizations for RAF callbacks
- Validate comprehensive test coverage passes
- Consider documenting the event propagation patterns
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting |
christian-byrne
pushed a commit
that referenced
this pull request
Oct 5, 2025
## Summary Snap link preview to the nearest compatible slot while dragging in Vue Nodes mode, and complete the connection on drop using the snapped target. Mirrors LiteGraph’s first-compatible-slot logic for node-level snapping and reuses the computed candidate for performance. ## Changes - Snap preview end to compatible slot - slot under cursor via `data-slot-key` fast-path - node under cursor via `findInputByType` / `findOutputByType` - Render path - `slotLinkPreviewRenderer.ts` now renders to `state.candidate.layout.position` - Complete on drop - Prefer `state.candidate` (no re-hit-testing) - Fallbacks: DOM slot → node first-compatible → reroute - Disconnects moving input link when dropped on canvas ## Review Focus - UX feel of snapping and drop completion (both directions) - Performance on large graphs (mousemove path is O(1) with dataset + single validation) - Edge cases: reroutes, moving existing links, collapsed nodes ## Screenshots (if applicable) https://github.com/user-attachments/assets/fbed0ae2-2231-473b-a05a-9aaf68e3f820 #5780 (snapping) <-- #5898 (drop on canvas + linkconnectoradapter refactor) <-- #5903 (fix reroute snapping) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5780-feat-vue-nodes-snap-link-preview-connect-on-drop-27a6d73d365081d89c8cf570e2049c89) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <[email protected]>
benceruleanlu
commented
Oct 7, 2025
Summary - Route LGraphNode slot position queries through a single source of truth that prefers layoutStore’s DOM‑tracked slot positions and falls back to geometry when unavailable. - Ensures that reroute‑origin drags snap to the same visual slot centers used by Vue nodes when hovering compatible nodes. What changed - LGraphNode.getInputPos/getOutputPos now resolve via getSlotPosition(), which: - Returns the Vue nodes layoutStore position if the slot has been tracked. - Otherwise, computes from node geometry (unchanged fallback behavior). - LGraphNode.getInputSlotPos(input) resolves index→getSlotPosition() and retains a safe fallback when an input slot object doesn’t map to an index. Why - Previously, when starting a drag from a reroute and hovering a node, the temporary link endpoint would render toward LiteGraph’s calculated slot position, not the Vue‑tracked slot position, causing a visible mismatch. - By making all slot position lookups consult the layout store first, node hover snap rendering is consistent across slot‑origin and reroute‑origin drags. Impact - No behavior change for non‑Vue nodes mode or when no tracked layout exists — the legacy calculated positions are still used. - Centralizes slot positioning in one place, reducing special‑case logic and making hover/drag rendering more predictable. #5780 (snapping) <-- #5898 (drop on canvas + linkconnectoradapter refactor) <-- #5903 (fix reroute snapping) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5903-Use-layoutStore-slot-positions-for-node-slot-queries-fix-reroute-origin-node-snap-2816d73d36508184b297f46b39105545) by [Unito](https://www.unito.io)
dosubot
bot
added
size:XXL
dosubot
bot
added
size:XL
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/08/2025, 02:04:21 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
benceruleanlu
commented
Oct 7, 2025
src/renderer/extensions/vueNodes/composables/useSlotLinkInteraction.ts
Outdated
Show resolved
Hide resolved
src/renderer/extensions/vueNodes/composables/useSlotLinkInteraction.ts
Outdated
Show resolved
Hide resolved
|
needs new snapshots after the merging of slot dimming, I made a mistake in the merging conflict resolution process |
…ing node.closest DOM read
dosubot
bot
added
size:XXL
benceruleanlu
added
New Browser Test Expectations
arjansingh
pushed a commit
that referenced
this pull request
Oct 7, 2025
## Summary Snap link preview to the nearest compatible slot while dragging in Vue Nodes mode, and complete the connection on drop using the snapped target. Mirrors LiteGraph’s first-compatible-slot logic for node-level snapping and reuses the computed candidate for performance. ## Changes - Snap preview end to compatible slot - slot under cursor via `data-slot-key` fast-path - node under cursor via `findInputByType` / `findOutputByType` - Render path - `slotLinkPreviewRenderer.ts` now renders to `state.candidate.layout.position` - Complete on drop - Prefer `state.candidate` (no re-hit-testing) - Fallbacks: DOM slot → node first-compatible → reroute - Disconnects moving input link when dropped on canvas ## Review Focus - UX feel of snapping and drop completion (both directions) - Performance on large graphs (mousemove path is O(1) with dataset + single validation) - Edge cases: reroutes, moving existing links, collapsed nodes ## Screenshots (if applicable) https://github.com/user-attachments/assets/fbed0ae2-2231-473b-a05a-9aaf68e3f820 #5780 (snapping) <-- #5898 (drop on canvas + linkconnectoradapter refactor) <-- #5903 (fix reroute snapping) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5780-feat-vue-nodes-snap-link-preview-connect-on-drop-27a6d73d365081d89c8cf570e2049c89) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <[email protected]>
arjansingh
pushed a commit
that referenced
this pull request
Oct 7, 2025
Snap link preview to the nearest compatible slot while dragging in Vue Nodes mode, and complete the connection on drop using the snapped target. Mirrors LiteGraph’s first-compatible-slot logic for node-level snapping and reuses the computed candidate for performance. - Snap preview end to compatible slot - slot under cursor via `data-slot-key` fast-path - node under cursor via `findInputByType` / `findOutputByType` - Render path - `slotLinkPreviewRenderer.ts` now renders to `state.candidate.layout.position` - Complete on drop - Prefer `state.candidate` (no re-hit-testing) - Fallbacks: DOM slot → node first-compatible → reroute - Disconnects moving input link when dropped on canvas - UX feel of snapping and drop completion (both directions) - Performance on large graphs (mousemove path is O(1) with dataset + single validation) - Edge cases: reroutes, moving existing links, collapsed nodes https://github.com/user-attachments/assets/fbed0ae2-2231-473b-a05a-9aaf68e3f820 #5780 (snapping) <-- #5898 (drop on canvas + linkconnectoradapter refactor) <-- #5903 (fix reroute snapping) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5780-feat-vue-nodes-snap-link-preview-connect-on-drop-27a6d73d365081d89c8cf570e2049c89) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <[email protected]>
benceruleanlu
added
New Browser Test Expectations
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements droponcanvas functionality and a linkconnectoradapter refactor.
There are some followup PRs that will fix/refactor some more noncritical things, like the terrible slotid, the number/string nodeid confusion, etc.
#5780 (snapping) <-- #5898 (drop on canvas + linkconnectoradapter refactor) <-- #5903 (fix reroute snapping)